test(etdump): Add FileDataSink test coverage #9332
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9332
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8e8c978 with merge base 8c32da7 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
@pytorchbot label "topic: not user facing" |
devtools/CMakeLists.txt
Outdated
| @@ -233,5 +235,5 @@ install( | |||
|
|
|||
| if(BUILD_TESTING) | |||
| # TODO: This is currently not working! | |||
There was a problem hiding this comment.
If you have brought the test building back, plz remove this comment
| @@ -45,16 +49,23 @@ class ProfilerETDumpTest : public ::testing::Test { | |||
| const size_t buf_size = 512 * 1024; | |||
| buf = (uint8_t*)malloc(buf_size * sizeof(uint8_t)); | |||
| etdump_gen[1] = new ETDumpGen(Span<uint8_t>(buf, buf_size)); | |||
|
|
|||
There was a problem hiding this comment.
It would be great if you can use the temp file et provided. You can find example here: https://github.com/pytorch/executorch/blob/main/extension/data_loader/test/file_data_loader_test.cpp#L15
Or is there any reason why you didn't choose that?
There was a problem hiding this comment.
Thanks for the feedback!!
|
@Megan0704-1 thanks for taking this task! Overall looks great and left some comments. Also some of the cis are broken. |
Summary: - Extend test loops to cover FileDataSink scenarios (j=2 case) - Add temporary file cleanup in TearDown - Include file_data_sink.h, stdio.h and fstream in test sources - Update CMakeLists.txt to ensure proper linking Fixes pytorch#9165
- Migrate from tmpnam to executorch::extension::testing::TempFile for secure temporary file handling - Add missing namespace qualification for TempFile class - Use std::unique_ptr for proper RAII management - Remove manual file deletion since TempFile handles cleanup - Remove building sdk_etdump_test in devtools/CMakeLists.txt
Megan0704-1
force-pushed
the
etdump-file-data-sink-test-coverage
branch
from
59edeea to
8e8c978
Compare
|
@Megan0704-1 LGTM! Thanks for your contribution! I will stamp it after cis are passed. |
test(etdump): Add FileDataSink test coverage
Summary
Adds test validation for FileDataSink usage in ETDumpGen scenarios. Extends existing debug event tests to cover file-based data sinks.
Test Plan
Build & Run Tests:
# From build directory (cmake-out) make -j32 ctest -R sdk_etdump_tests --output-on-failureFixes #9165